not a big fan of this being a string an being parsed later on. If we have to do it this way, perhaps we could also pull it out to a constant? requeueImmediate or something?

it seemed like that as well for me. Here is the way it looks good to me. Could have those changes sin this PR if you prefer it better. But all of them should land soon so i decided to keep this one as is 🤔

and along with it there would be only one parse call and it is only being set from time.Duration.String() string so we could be sure it will succeed

Ok in that case I am ok to approve with the improvements that are coming

well, there is no harm in having them here I guess.

shoud we be ignoring the error here? Is it possible we have a bad value?

possible only if someone goes into the CR and changes values manually. This value is being set by .String() call on the duration struct. In case someone messes up with it will be 0 Duration. Then the logic later on will look like this:

expiryTime := previousReconcileStart.Add(0)
       // new reconcile will always be After expiryTime, so this clause always ends up as false 
	if !generationChanged(dnsRecord) && reconcileStart.Before(&expiryTime) {
               // will not requeue 
		return 0, nil
	}

and we won't use the requeueIn anywhere else so the zero duration doesn't matter.

In other words, we really don't care about an error there - user would need to manually play with it and if they do and set an invalid value we will end up ignoring it

nit: To me the logic around updating the conditions and deciding what this requeue time value should be is not the concern of applyChanges. This should just return an error and a bool to say if it had changes, and wherever this is being called use that info to calculate the status and requeue validation time.

It also looks like you are setting stuff in the status before you even know if it successfully updated or not.

hmm, a good point. I'll note it and will create a follow-up that, hopefully, won't die forgotten by people.
And I was trying to set the status properly 🤔 like even if we weren't successful in updating i still want the timestamp of the reconciliation that prompted an error or if gen has changed i still want to reset the counter regardless of registry failing to publish the plan 🤔 the questionable is the ValidFor field before the success of applied changes, but one could argue that if everything goes bad with the DNS Provider and we set ValidFor for 5 seconds there will be:

• a error message saying that "oh, boi, you are messed up"
  or
• b immediate reconciliation (even when i say not to requeue controller-runtime will requeue reconciliation if there will be an error returned from the .Reconcile() call)

This, actually, is a problem - we want at one point to give up reconciling but return an error, but it would keep reconciling. I opted for just leaving things as is (it would be an edge-ish case) instead of creating silly error assertion. It would do one more tinny loop, won't go into DNS logic and will stop, but that is another story.

But it is still a excelent suggestion, thank you! I guess what I was trying to say - the way it is now should not break anything, but you are correct - there is more elegant way to deal with it